Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: add findPackageJSON util #55412

Merged

Conversation

JakobJingleheimer
Copy link
Member

#55229 was too controversial, so back to the original idea from #55173.

Closes #55229

@JakobJingleheimer JakobJingleheimer added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 16, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 16, 2024
@JakobJingleheimer JakobJingleheimer added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 16, 2024
@ljharb
Copy link
Member

ljharb commented Oct 16, 2024

@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Oct 16, 2024

"Just" seems a little dismissive 😕

The first package was last updated 7 years ago (so it may well be outdated, plus its cited limitations).

The second package is deprecated.

The third is maybe similar/close; it appears to only do direct path manipulation and won't respect module resolution (ex from a loader hook), which this does respect.

Edit: looking closer, the third package has several limitations that this does not, such how it handles relative search (cwd vs current module location) and, AFAICS, it does not handle node_modules.

This package is also leveraging/mirroring node's internals, so it will behave exactly as node is doing internally (whereas userland may not, and would have to re-invent the wheel to do so).

@ljharb
Copy link
Member

ljharb commented Oct 16, 2024

apologies, you're right, a better way to phrase it is "is this the same capability as these packages".

Certainly doing this internally in node is better for tons of reasons, and I'm glad you're adding it :-) I wanted to confirm what the semantics were, is all.

@JakobJingleheimer
Copy link
Member Author

I wanted to confirm what the semantics were, is all.

Ah, yes then 🙂

@JakobJingleheimer
Copy link
Member Author

The markdown linter is complaining about a codeblock that has no language. It's a directory listing—language is not applicable (it just needs the default codeblock treatment).

Can I disable the rule for that block?

@ljharb
Copy link
Member

ljharb commented Oct 16, 2024

can you tag it as sh?

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 98.80478% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (53b1050) to head (ba9acf9).
Report is 237 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/package_json_reader.js 99.08% 2 Missing ⚠️
lib/internal/modules/cjs/loader.js 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55412      +/-   ##
==========================================
- Coverage   88.42%   88.41%   -0.01%     
==========================================
  Files         654      654              
  Lines      187552   187657     +105     
  Branches    36087    36117      +30     
==========================================
+ Hits       165839   165919      +80     
- Misses      14950    14972      +22     
- Partials     6763     6766       +3     
Files with missing lines Coverage Δ
lib/internal/modules/esm/resolve.js 96.17% <100.00%> (-0.16%) ⬇️
lib/module.js 100.00% <100.00%> (ø)
lib/internal/modules/cjs/loader.js 97.59% <90.90%> (-0.06%) ⬇️
lib/internal/modules/package_json_reader.js 99.40% <99.08%> (-0.60%) ⬇️

... and 34 files with indirect coverage changes

@JakobJingleheimer
Copy link
Member Author

can you tag it as sh?

That could hack around it, but it would enable shell syntax highlighting. There should be no syntax highlighting.

@targos
Copy link
Member

targos commented Oct 17, 2024

What about text? It's already used in many places of the docs.

@JakobJingleheimer
Copy link
Member Author

What about text? It's already used in many places of the docs.

Huzzah! perfect. I didn't know that was an option. Thanks!

doc/api/module.md Outdated Show resolved Hide resolved
lib/internal/modules/package_json_reader.js Outdated Show resolved Hide resolved
lib/internal/modules/package_json_reader.js Outdated Show resolved Hide resolved
doc/api/module.md Outdated Show resolved Hide resolved
test/parallel/test-find-package-json.js Outdated Show resolved Hide resolved
doc/api/module.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@JakobJingleheimer JakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024
@nodejs-github-bot
Copy link
Collaborator

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2024
PR-URL: #55412
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Nov 1, 2024
Notable changes:

doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) nodejs#55282

PR-URL: TODO
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55412
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add module.stripTypeScriptTypes (Marco Ippolito) #55282
tools:
  * fix root certificate updater (Richard Lau) #55681

PR-URL: TODO
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 6, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
@JakobJingleheimer JakobJingleheimer added backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 8, 2024
@JakobJingleheimer JakobJingleheimer added lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 8, 2024
aduh95 added a commit that referenced this pull request Nov 9, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 11, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
aduh95 added a commit that referenced this pull request Nov 11, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) #55681
doc:
  * move typescript support to active development (Marco Ippolito) #55536
  * add jazelly to collaborators (Jason Zhang) #55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) #55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) #55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) #55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) #55282

PR-URL: #55741
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55412
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) nodejs#55681
doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) nodejs#55282

PR-URL: nodejs#55741
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
Notable changes:

crypto:
  * update root certificates to NSS 3.104 (Richard Lau) nodejs#55681
doc:
  * move typescript support to active development (Marco Ippolito) nodejs#55536
  * add jazelly to collaborators (Jason Zhang) nodejs#55531
fs:
  * (SEMVER-MINOR) make `dirent.path` writable (Antoine du Hamel) nodejs#55547
http:
  * (SEMVER-MINOR) add diagnostic channel `http.client.request.created` (Marco Ippolito) nodejs#55586
module:
  * (SEMVER-MINOR) add `findPackageJSON` util (Jacob Smith) nodejs#55412
  * (SEMVER-MINOR) add `module.stripTypeScriptTypes` (Marco Ippolito) nodejs#55282

PR-URL: nodejs#55741
@ruyadorno
Copy link
Member

This commit does not land cleanly on v22.x-staging and will need manual backport in case we want it in v22.x.

@ruyadorno ruyadorno added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Nov 27, 2024
@JakobJingleheimer
Copy link
Member Author

@ruyadorno is this something I can help with? Would be great to get this in the pending 22.x release.

PS sorry I didn't get a notification for your message.

@aduh95
Copy link
Contributor

aduh95 commented Nov 29, 2024

@JakobJingleheimer yes, you can open a backport PR that cherry-picks adda37f on top of v22.x-staging

JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this pull request Nov 29, 2024
PR-URL: nodejs#55412
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95 aduh95 added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. lts-watch-v22.x PRs that may need to be released in v22.x labels Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-open-v22.x Indicate that the PR has an open backport commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lts-watch-v20.x PRs that may need to be released in v20.x module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants